Skip to content

[#378] Fix trade indexing: remove missing user_address column#379

Closed
realproject7 wants to merge 1 commit intomainfrom
task/378-trade-indexing-fix
Closed

[#378] Fix trade indexing: remove missing user_address column#379
realproject7 wants to merge 1 commit intomainfrom
task/378-trade-indexing-fix

Conversation

@realproject7
Copy link
Copy Markdown
Owner

Summary

  • Root cause: trade_history table lacks user_address column, causing all trade upserts to fail silently with PostgREST error
  • Fix: omit user_address from insert in both /api/index/trade and /api/cron/trade-history
  • Follow-up needed: add user_address TEXT column to trade_history via Supabase migration, then restore the field in the insert

Test plan

  • POST trade tx → indexed: N where N > 0 (currently returns indexed: 0)
  • trade_history rows appear in Supabase after indexing
  • Cron trade-history route also works

Fixes #378

🤖 Generated with Claude Code

The trade_history table does not have a user_address column, causing
all trade upserts to fail silently with a PostgREST schema cache error.
Omit user_address from both the API route and cron trade-history route
so trades can be indexed. The column should be added via a Supabase
migration in a follow-up, then restored in the insert.

Fixes #378

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

T2b Review — REQUEST CHANGES

The root cause diagnosis is correct (PostgREST silently rejects inserts with unknown columns). However, omitting user_address has two downstream consequences:

1. V3.7 in e2e-verify.ts will fail

The verification script checks trade.user_address === results.deployer.toLowerCase() (V3.7). With user_address omitted, this field will be null in Supabase and V3.7 will always fail. Either skip/update V3.7 in the verification script, or add the migration in this PR.

2. Data loss on production trades

user_address tracks who executed the trade — important for user portfolios and trade attribution. Omitting it means all trades indexed before the migration lands will permanently lack this data (unless backfilled).

Recommendation: Add the Supabase migration (ALTER TABLE trade_history ADD COLUMN user_address text;) in this PR rather than deferring it. That way both routes can keep inserting user_address and no data is lost. If you can't run migrations from here, at minimum update e2e-verify.ts V3.7 to handle null user_address.

Copy link
Copy Markdown
Collaborator

@project7-interns project7-interns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES

Summary

This patch would unblock inserts, but it does so by dropping user_address from trade writes entirely. That regresses a field the current E2E verification explicitly checks, so it does not fully satisfy the trade-indexing acceptance criteria.

Findings

  • [high] Removing user_address from trade inserts makes V3.7 fail by design instead of fixing the trade path end-to-end.

    • File: src/app/api/index/trade/route.ts:96
    • Suggestion: add the missing user_address column via migration and keep persisting it, rather than dropping the field from the write path. The current verification script still checks trade.user_address === deployer (scripts/e2e-verify.ts:595), so this workaround only trades the existing V3.2 failure for a guaranteed V3.7 failure.
  • [medium] The same regression is duplicated in the cron backfill path, so even after a later migration, historical trades reprocessed through cron would still lose user attribution unless this route is revisited.

    • File: src/app/api/cron/trade-history/route.ts:203
    • Suggestion: keep the backfill path aligned with the API route and land the schema migration as part of this fix set.

Decision

Requesting changes because the proposed workaround persists rows by dropping required trade attribution, which leaves the E2E trade verification incomplete.

@realproject7
Copy link
Copy Markdown
Owner Author

Resolved by applying existing migration 00019_trade_history_user_address.sql — no code change needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Trade Indexer Returns 200 But Persists No Trade History Records

2 participants